quic: Implement callback for http/3 settings/application options#63558
quic: Implement callback for http/3 settings/application options#63558martenrichter wants to merge 6 commits into
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
martenrichter
force-pushed
the
http3settings
branch
3 times, most recently
from
d6acf53 to
eb500d1
Compare
Implements a callback that is invoked once http/3 settings are received. Background, http/3 settings usually arrive a bit later than connection establishment, and e.g. for webtransport these settings are used to indicate support. So e.g. the examples for quiche from google, wait for the settings to arrive. (This is different to http/2). The implemented callback mechanism allows to wait for the settings to arrive until connection attempts are made. As settings are stored in the generic applications option object, the callback's name refers to the application rather than the settings. Whether this is a good choice is debatable. Fixes: nodejs#63553 Signed-off-by: Marten Richter <marten.richter@freenet.de>
martenrichter
force-pushed
the
http3settings
branch
from
eb500d1 to
12245aa
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63558 +/- ##
==========================================
+ Coverage 90.14% 90.34% +0.19%
==========================================
Files 718 730 +12
Lines 227984 234547 +6563
Branches 42835 43909 +1074
==========================================
+ Hits 205522 211906 +6384
- Misses 14235 14383 +148
- Partials 8227 8258 +31
🚀 New features to boost your workflow:
|
|
OK, I am ready, so can someone from you push it? |
bjohansebas
added
request-ci
|
@nodejs/quic |
github-actions
Bot
removed
the
request-ci
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Thanks @martenrichter! This'll definitely be useful.
I'd prefer to change the name, I could be persuaded there if others disagree though.
The other comments are a couple of clear little bugs we should definitely fix.
| The endpoint that created this session. Returns `null` if the session | ||
| has been destroyed. Read only. | ||
|
|
||
| ### `session.onapplication` |
There was a problem hiding this comment.
The naming here is confusing, as you mentioned in the description, since it doesn't emit an application as such. This isn't a generic QUIC thing at all (transport parameters are a separate thing with a different flow) and right now this won't work for anything except HTTP/3. While some other protocols have equivalents it's not clear how closely they would all map to the model, and they definitely all have different names & structures anyway.
For HTTP/3, the RFC calls those 'settings', we use localSettings/remoteSettings in our existing HTTP/2 API. Since this is basically HTTP only and there's no umbrella term, I'd suggest this say 'settings' somewhere explicitly. Just onsettings would work fine from my pov. The rest of our HTTP-specific stuff (onheaders) doesn't explicitly name HTTP in the API naming so that seems consistent.
Beyond this PR: the HTTP/QUIC ambiguity here in the API ties a bit into my thoughts about the request timeout stuff @jasnell - starting to wonder whether we should separate the QUIC & HTTP/3 APIs more explicitly, to make this kind of thing cleaner. The distinction is very blurred together right now and we do lots of HTTP-shaped things for QUIC. Looks doable to split it a bit, really just an API restructuring rather than internals, but there'd be plenty of details to work out. Not required for this PR at all, just fyi, when I have time next week I'll take a look around it as a more general topic and try to open a rough draft to discuss, I have some ideas that'd be fun to explore 😄.
There was a problem hiding this comment.
The problem with the naming is, that it dates back to the choice that the http/3 settings are all inside the quic.ApplicationOptions . So I had to decide to call it onsettings , which would be my preferred choice for http/3 settings, or stick with the generic term applicationoptions (though I shortened it to onapplication as it was already very long).
So I think it is kind of a general decision. I have so far just flipped a coin.
Regarding the HTTP/QUIC ambiguity, I am playing around with how to implement webtransport and ngtcp2, and I think you can even wrap the WT streams to the QuicStream and have a mixture of streams. Even if it is confusing, it works some way.
P.S: I try to cover the minor stuff. As I do it in my free time, and the holiday day today is over it may take until next weekend.
There was a problem hiding this comment.
Minor stuff should be all covered. I am now waiting for consensus on the naming scheme.
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Fixes look good. I'd prefer something with 'settings' in the name, but happy to merge as is & change it later if there's no immediate consensus.
|
Let's see if someone else has an opinion. (I do not have a one...., of course I am happy with no renaming work yet). |
6 participants
Implements a callback that is invoked once http/3 settings are received.
Background, http/3 settings usually arrive a bit later than connection establishment, and e.g. for
webtransport these settings are used to indicate support. So e.g. the examples for quiche from google, wait for the settings to arrive. (This is different to http/2).
The implemented callback mechanism allows to wait for the settings to arrive until connection attempts are made.
As settings are stored in the generic applications option object, the callback's name refers to the application rather than the settings.
Whether this is a good choice is debatable.
Fixes: #63553